The c3i_test channel may have some missing packages.

Codecov Report

Merging #21813 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #21813      +/-   ##
==========================================
+ Coverage   91.99%      92%   +<.01%     
==========================================
  Files         167      170       +3     
  Lines       50578    50553      -25     
==========================================
- Hits        46530    46510      -20     
+ Misses       4048     4043       -5

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0828c25...5d40eae. Read the comment docs.

Well, good news is we got a repro of this on our CI - it's not a python 3.7 issue, but instead a Visual Studio 2017 one.
https://ci.appveyor.com/project/pandas-dev/pandas/build/1.0.15961/job/r9axp415no6jubnr#L2059

Still not convinced this isn't a compiler bug, but having trouble to get a simple case to reproduce - one option would be to build our wheels VS 2015 - but annoying, b/c python forces the newest version, so can't have 2017 installed. I'll keep trying some things.

This is kind of horrible, but it does fix the problem (just a lint issue I pushed for) and I'm out of ideas - any thoughts or suggestions @jreback, @TomAugspurger ?

@chris-b1 could you summarize the issue? You think MSVC is just optimizing away if val == val, which isn't true when val is NaN?

That's right, optimizing maybe not the right word, but somehow the add_var function is being turned into a no-op, the mean_x, ssqdm_x, and nobs variables don't get updated. Adding any kind of unconditional action to the function fixes it - originally discovered by noticing it works with a print statement thrown in.

And yes, val==val is a not-a-NaN check. We use it all over the place in this module, so it's not as if that check is universally being treated wrong, but something about this particular context makes MSVC think it's unreachable or something.

Ok, thanks...

@jjhelmus do we know anyone at Microsoft who works on their compilers that might be able to provide insight on #21813 (comment) and #21813 (comment)?

@zooba might be able to connect you to the right team at Microsoft

I can try, but I haven't been having as much luck recently.

Since this is Cython, can someone post the generated code for the comparison? There's a much better chance of getting something useful if they're looking at the code they actually compile, and not the code someone else is compiling.

It's Cython, so of course the bigger file is a tangled mess, but the particular function is easy to read - lines of gist here.
https://gist.github.com/chris-b1/cd974fb6b4165285a48b6d6c12129b40#file-window-cpp-L11028-L11096

Called in a few places, but for instance, here is a problem one.
https://gist.github.com/chris-b1/cd974fb6b4165285a48b6d6c12129b40#file-window-cpp-L11948

Cython isn't doing anything particular interesting - it's a cdef function so basically translates line for line into c++.

I can post it later, not on this machine, but I've also tried to write a simpler, plain c++ example that triggers the problem, but it always worked - really not sure why.

I can reproduce it with this snippet (has a few tricks to try and defeat optimisations, but they are likely unnecessary):

#include <math.h>
#include <stdio.h>

double get_value(int i) {
    return i > 1 ? NAN : 7;
}

int check_default(double v1, double v2) {
    return ((v1 == v2) != 0);
}

#pragma float_control(precise, off, push)
int check_fast(double v1, double v2) {
    return ((v1 == v2) != 0);
}
#pragma float_control(pop)


int main(int argc, char **argv) {
    double x1 = get_value(argc);
    double x2 = get_value(argc);
    fprintf(stdout, "default: %d\nfast:    %d\n",
        check_default(x1, x2), check_fast(x1, x2)
    );
}

The comparison doesn't behave properly when you disable precise floating point operations. You may be doing this in the build with /fp:fast (if you build this snippet with that option, both checks return '1'; if you build with the default /fp:precise, it works). In fact, the docs for this flag specifically call out NaN comparisons as something that only work in precise mode.

So I'd guess you need to update the compiler flags for this one, or possibly add an explicit isnan() call.

Is the MSVC info something that can be inferred from platform.uname (at compile-time)?

Thanks for taking a look at this @zooba - unfortunately that's not exactly the same problem I'm seeing here, the compiled code is acting as if (v1 == v2) always evaluates to False/0. We are building with /fp:precise (via defaults, but I've also tried manually overriding)

Interestingly using isnan from <math.h> does solve the problem here - still feels like MSVC is doing something wrong in the former case, but that's a reasonable enough workaround.

the compiled code is acting as if (v1 == v2) always evaluates to False/0

Do you have a disassembly showing this? This would be very strange to see as a compiler optimization (unless you mean (v == v) always evaluates to True/1?)

b/c python forces the newest version, so can't have 2017 installed

Just saw this while rereading the thread. If you start with all the tools on PATH and set DISTUTILS_USE_SDK=1 then it'll skip the autodetection and use whatever you provide. (And if you help encourage the ecosystem to get off distutils completely, then more of these problems go away :) )

Could this SO answer be relevant?

https://stackoverflow.com/questions/8200311/checking-for-nan-in-cython/20031818#20031818

@zooba - do you know how to pull symbols into the dissambly from a .pyd? I was playing around with dumpbin / compile flags, but never got anything better than, e.g. below, where everything is still in offsets.

 dumpbin.exe /DISASM pandas\_libs\window.cp36-win_amd64.pyd /OUT:tmp.txt

do you know how to pull symbols into the dissambly from a .pyd

Answering my own question - I was missing '/DEBUG' on extra_link_args

Alright @zooba - I think I have this to a reasonable bit of a dis-assembly - pushing the bounds of my knowledge but looks like a code-generation bug.

Here's MSVC 2015 (printfs surround a call to add_var) which works correctly. In particular the highlighted lines are the NaN check.
https://gist.github.com/chris-b1/1049fdabafc56b2a25de7ad377016baa#file-msvc2015-asm-L6-L8

And here is MSVC 2017. The function call doesn't get inlined - half of the NaN check gets done (jn) - then the stack is being set up, then the other half the check (jne). I believe in setting up the stack, the ZF flag originally set by ucomisd gets set back to 0 (from the sub) so the jne check will always be true, and the function unconditionally exits early.
https://gist.github.com/chris-b1/199f818c1fbb8f8c4108aa9e430df4f7#file-msvc2017-asm-L15-L19

@chris-b1 Yep, you're absolutely right. Thanks for tracking that down, I'll pass it on!

Here is the public side of the report: https://developercommunity.visualstudio.com/content/problem/294290/incorrect-codegen-for-double-equality.html Feel free to add pings there whenever you like

Thanks @zooba!

and thanks @jbrockmendel - that's what I'll have to do to fix the 2.7 build, more modern MSVCs are basically standards compliant and have isnan where it should be.

does using the fix proposed in #21940 help here? e.g. npy_isnan?

does using the fix proposed in #21940 help here? e.g. npy_isnan?

It does (latest commit has that and is passing on appveyor) - but less sure we want to apply that universally, I'll try perf on @jbrockmendel PR later, suspect it may be worse on Windows.

@chris-b1 this looks good. can you rebase once more.

perf looks ok? (even if its slightly worse this is a bug :>)

Go ahead and merge on green.

@jreback thanks, yeah perf will be fine, just slightly slower on Windows for this particular function

There seem to be a conflict, please backport manually

cc @chris-b1 do you want to see if you can backprot to 23.x branch?